fix(node): prevent uncatchable crash when canceling a request body stream - #76
Conversation
…ream A consumer canceling a streamed request body mid-upload could crash the whole process: Node's Readable.toWeb adapter enqueues from 'data' events, so a chunk arriving after cancel hits an already-closed controller and throws an uncaught ERR_INVALID_STATE (nodejs/node#54205). Route all Readable-to-web conversions in body and event-stream through a new cancel-safe toWebReadableStream util, which pipes the adapter through a TransformStream so cancellation never reaches its controller mid-chunk, and copies each chunk out of Node's pooled Buffer memory. Pinned by flood-and-abort upload tests over http1 and http2, including tests documenting that the bare adapter still crashes.
@standardserver/aws-lambda
@standardserver/core
@standardserver/fastify
@standardserver/fetch
@standardserver/node
@standardserver/peer
@standardserver/shared
commit: |
Merging this PR will not alter performance
Comparing 
Footnotes
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
The TransformStream wrap does not reliably prevent the crash, and toWebReadableStream's own unit test currently fails: utils.test.ts → does not throw when a raw buffer stream is cancelled mid-read deterministically leaks an uncatchable ERR_INVALID_STATE ("Controller is already closed") on Node v24.18.0.
Reviewed changes
- Cancel-safe
toWebReadableStreamutil — new wrapper inpackages/node/src/utils.ts:15that pipesReadable.toWeb(stream)through aTransformStream, copying each chunk into a freshUint8Array. - Routed all node request-body stream conversions through it —
body.ts(toStandardBody, octet-stream path) andevent-stream.ts(toAsyncIteratorObject) no longer call bareReadable.toWeb(req). - Flood-and-abort tests — http1/http2 upload servers; a "documents the bug" bare
Readable.toWebnegative test and a "keeps ... from crashing" wrapped positive test per protocol, plus unit tests for byte fidelity, copy semantics, and mid-read cancellation.
DeepSeek Flash (default — pick a model for stronger reviews) ｜ 𝕏
The Readable.toWeb + TransformStream approach only shielded the adapter's controller on some Node releases: on Node 24 a chunk arriving after cancel still hit the closed controller and crashed the process with an uncaught ERR_INVALID_STATE, even for plain Readable sources. Hand-roll the adapter instead of wrapping Readable.toWeb: chunks are only enqueued inside pull(), which never runs after cancel, so the crash is impossible by construction regardless of Node's adapter internals. Cancel destroys the source, except http1 server requests — they share their socket with the response, so they are abandoned (stalled by backpressure, reclaimed on connection teardown) to keep in-flight responses deliverable. All package and e2e suites pass on Node 22, 24, and 26.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Replaced the
TransformStreamwrap with a hand-rolled pull-basedReadableStreamintoWebReadableStream(packages/node/src/utils.ts) that reads the source via its async iterator, so chunks are only enqueued insidepulland never after a consumer cancel — eliminating theERR_INVALID_STATEprocess crash by construction. - Reworked
cancel(reason)to tear the source down withstream.destroy(reason), except http1 serverIncomingMessages (shared socket with the response), which are deliberately abandoned to keep in-flight responses deliverable. - Updated the
toWebReadableStreamunit/flood-and-abort tests; the mid-read cancel test that previously failed deterministically on Node 24 now passes.
The prior review's critical concern (mid-read cancel leaking an uncatchable ERR_INVALID_STATE on Node v24.18.0 through the TransformStream wrap) is fully addressed and verified: I ran packages/node tests on this exact Node version — the does not throw when a raw buffer stream is cancelled mid-read test and both "keeps an aborted upload from crashing the process" flood tests pass, and the full 73-test suite is green.
DeepSeek Flash (default — pick a model for stronger reviews) ｜ 𝕏
1 participant
Canceling a streamed request body mid-upload (e.g. rejecting an oversized upload while the client is still sending) could crash the entire server process. Node's
Readable.toWebadapter enqueues from'data'events, so a chunk arriving after cancel hits an already-closed controller and throws an uncatchableERR_INVALID_STATE(nodejs/node#54205) — notry/catchin user code can reach it. Wrapping the adapter in aTransformStreamturned out to shield the crash only on some Node releases (fine on 22 and 26, still crashed on 24), so the final fix dropsReadable.toWebentirely.Fixes
body.tsandevent-stream.tsnow convert via a hand-rolled pull-basedtoWebReadableStreamutil: chunks are only enqueued insidepull(), which never runs after cancel, so the crash is impossible by construction regardless of Node's adapter internals.Uint8Arraychunks backed by exact-sized buffers instead of pooled NodeBuffers, so touchingchunk.buffercan no longer expose unrelated pool memory.Testing
Readable.toWebstill crashes under the same load, so they double as the signal for when the workaround can be dropped.bodyandevent-streamtests assert the returned stream is exactly the wrapper's output; unit tests cover byte fidelity, chunk copying, and mid-read cancellation.